Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add structure to foi_index across the package #221

Open
wants to merge 5 commits into
base: dev
Choose a base branch
from
Open

Add structure to foi_index across the package #221

wants to merge 5 commits into from

Conversation

ntorresd
Copy link
Member

As pointed out by @ben18785 in #200, the way in which we were indexing the FOI was ambiguous as we were just providing a vector with the indexes (see #206):

...
foi_index <- rep(c(1, 2, 3), c(25, 10, 15))
seromodel_time_normal <- fit_seromodel(
  serosurvey = serosurvey,
  model_type = "time",
  foi_index = foi_index
)
foi_index
[1] 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 2 2 2 2 2 2 2 2 2 2 3 3 3 3 3 3 3 3 3 3 3 3 3 3 3

rather than an structured object to specify the age/time indexation for estimating the FOI:

...
foi_index <- data.frame(
  year = seq(2000, 2049),
  foi_index = rep(c(1, 2, 3), c(25, 10, 15))
)
seromodel_time_normal <- fit_seromodel(
  serosurvey = serosurvey,
  model_type = "time",
  foi_index = foi_index
)
glimpse(foi_index)
Rows: 50
Columns: 2
$ year      <int> 2000, 2001, 2002, 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011, 2012, 2013, 2014,…
$ foi_index <dbl> 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 2, 2, 2, 2, 2,

Now on, users will need to specify foi_index as a data.frame indexing either ages or years (as shown above) rather than as an unstructured list. get_foi_index() may still be used for this end and its used the same as before with the caveat that the model type must be explicitly specified (e.g.):

data(chagas2012)
foi_index <- get_foi_index(
  chagas2012,
  group_size = 25,
  model_type = "time"
)
glimpse(foi_index)
Rows: 79
Columns: 2
$ year      <int> 1936, 1937, 1938, 1939, 1940, 1941, 1942, 1943, 1944, 1945, 1946, 1947, 1948, 1949, 1950,…
$ foi_index <dbl> 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 2, 2, 2, 2, 2,…

This PR also adds unit tests for get_foi_index(), addressing #220.

@ntorresd ntorresd changed the base branch from main to dev September 26, 2024 00:00
@ntorresd ntorresd marked this pull request as ready for review September 26, 2024 00:01
ben18785
ben18785 requested changes Oct 1, 2024
View reviewed changes
Copy link
Collaborator

@ben18785 ben18785 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks very much, @ntorresd -- this is so much better. A few minor changes then I'm happy for this to go in.

R/build_stan_data.R Show resolved Hide resolved
R/build_stan_data.R Show resolved Hide resolved
R/validation.R
@@ -107,3 +107,28 @@ validate_survey_and_foi_consistency_age_time <- function( #nolint
"not exceed max age in survey_features."
)
}

validate_foi_index <- function(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest checking that the FOI chunks are consecutive: e.g. that we don't get 1, 1, 1, 2, 2, 2, 1, 1, 3, 3. I think we also need to have indices for all integers up until the max. So we would raise a warning for 1, 1, 1, 3, 3 for instance because it misses a 2.

tests/testthat/test-get_foi_index.R
names(result_age),
must.include = c("age", "foi_index")
)
expect_equal(nrow(result_age), max(survey_features$age_max))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there are more things we could check: e.g. max index, all indices represented up until that max index, that we only have contiguous indices. I realise this may seem overkill given the simplicity of this function, but we could imagine changing this function in the future and we'd want to check that it still works as intended.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ben18785 ben18785 ben18785 requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ntorresd @ben18785